Automated Test: improve-two-factor-authentication-features #369
+280
−36
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Peer Richelsen <peeroke@gmail.com>
📝 WalkthroughWalkthroughThis PR introduces backup codes as a secondary authentication method for two-factor authentication. Users can generate backup codes during 2FA setup, use them to disable 2FA without their authenticator, and authenticate during login. The feature spans UI components, authentication flows, database schema, and error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Client as Client (Web)
participant Server as Server
participant DB as Database
User->>Client: 1. Initiate 2FA setup
Client->>Server: POST /totp/setup
Server->>Server: Generate secret & 10 backup codes
Server->>Server: Encrypt backup codes with CALENDSO_ENCRYPTION_KEY
Server->>DB: Save encrypted backupCodes, secret
Server-->>Client: Return secret, keyUri, dataUri, backupCodes
Client->>User: Display QR code, then DisplayBackupCodes step
User->>Client: Download/Copy backup codes
User->>Client: Confirm setup complete
Client->>Server: POST /totp/enable (verify QR scan)
Server->>DB: Confirm twoFactorEnabled: true
Server-->>Client: Success
rect rgba(200, 150, 255, 0.5)
Note over User,DB: Backup Code Usage
end
User->>Client: Lost access to authenticator
Client->>Client: Toggle twoFactorLostAccess state
Client->>User: Show BackupCode input
User->>Client: Enter backup code
Client->>Server: POST credentials with backupCode
Server->>Server: Decrypt user.backupCodes
Server->>Server: Validate code, mark as used (set to null)
Server->>Server: Re-encrypt remaining codes
Server->>DB: Update user with invalidated code
Server-->>Client: Return JWT (authenticated)
Client-->>User: Login success
sequenceDiagram
participant User as User
participant Client as Client (Web)
participant Server as Server
participant DB as Database
rect rgba(255, 100, 100, 0.5)
Note over User,DB: Disable 2FA with Backup Code
end
User->>Client: Click "Disable 2FA"
Client->>User: Show password + TOTP input
User->>Client: Can't access authenticator
User->>Client: Click "Lost Access"
Client->>Client: Toggle twoFactorLostAccess, show BackupCode
User->>Client: Enter password + backup code
Client->>Server: POST /disable with password, backupCode
Server->>Server: Decrypt user.backupCodes
Server->>Server: Validate backup code against decrypted list
alt Backup code valid
Server->>DB: Update user: twoFactorEnabled: false, backupCodes: null
Server-->>Client: Success
Client-->>User: 2FA disabled
else Backup code invalid
Server-->>Client: Error (IncorrectBackupCode)
Client-->>User: Show error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/web/pages/api/auth/two-factor/totp/disable.ts`:
- Around line 58-63: Normalize the incoming backup code before lookup: take
req.body.backupCode, trim surrounding whitespace, remove internal whitespace and
dashes (e.g., replace spaces and "-" characters), convert to the same case used
in stored backupCodes (e.g., toLowerCase()), then check indexOf against
backupCodes; update the lookup that currently does
backupCodes.indexOf(req.body.backupCode.replaceAll("-", "")) to use this
normalized value and keep the existing error return
(ErrorCode.IncorrectBackupCode) unchanged.
In `@apps/web/playwright/login.2fa.e2e.ts`:
- Around line 50-54: The current assertion uses
page.locator('[data-testid=two-factor-switch]').isChecked() which returns a
Promise<boolean> and is not awaited, so the test asserts the Promise object
itself; change the assertion to either await the boolean (await
page.locator('[data-testid=two-factor-switch]').isChecked() and assert the
result) or — preferably — use the Playwright locator matcher on the locator
(e.g., expect(page.locator('[data-testid=two-factor-switch]')).toBeChecked()) to
get proper awaiting and auto-retry behavior.
In `@packages/ui/components/form/inputs/Input.tsx`:
- Around line 47-51: The password visibility toggle button in Input.tsx is
currently removed from keyboard focus by tabIndex={-1}; update the button (the
element that calls toggleIsPasswordVisible()) to be keyboard-focusable by
removing the tabIndex or setting it to 0, and ensure it has an accessible name
(e.g., aria-label or visible text) so keyboard and screen-reader users can
operate the toggle.
🧹 Nitpick comments (2)
packages/features/auth/lib/next-auth-options.ts (1)
131-155: Normalize and prune backup codes after use (robustness).Trimming/lowercasing prevents false negatives on pasted codes, and pruning used codes avoids persisting dead entries and lets you null out the field when exhausted.
♻️ Suggested tweak
- const backupCodes = JSON.parse( - symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY) - ); + const backupCodes = JSON.parse( + symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY) + ) as Array<string | null>; + + const normalizedBackupCode = credentials.backupCode.replaceAll("-", "").trim().toLowerCase(); - const index = backupCodes.indexOf(credentials.backupCode.replaceAll("-", "")); + const index = backupCodes.indexOf(normalizedBackupCode); if (index === -1) throw new Error(ErrorCode.IncorrectBackupCode); - backupCodes[index] = null; + backupCodes[index] = null; + const remainingBackupCodes = backupCodes.filter((code): code is string => !!code); await prisma.user.update({ where: { id: user.id, }, data: { - backupCodes: symmetricEncrypt(JSON.stringify(backupCodes), process.env.CALENDSO_ENCRYPTION_KEY), + backupCodes: remainingBackupCodes.length + ? symmetricEncrypt(JSON.stringify(remainingBackupCodes), process.env.CALENDSO_ENCRYPTION_KEY) + : null, }, });apps/web/components/settings/EnableTwoFactorModal.tsx (1)
64-65: Clear backup‑code artifacts on reset/close.
The download blob URL and backup‑code data stay in memory after the modal closes, which leaves sensitive data and an object URL lingering. Consider clearing state and revoking the URL when the flow ends.♻️ Suggested cleanup
const resetState = () => { setPassword(""); setErrorMessage(null); setStep(SetupStep.ConfirmPassword); + setBackupCodes([]); + setDataUri(""); + setSecret(""); + if (backupCodesUrl) { + URL.revokeObjectURL(backupCodesUrl); + setBackupCodesUrl(""); + } };Also applies to: 71-75, 92-100, 261-287
Comment on lines
+58
to
+63
| const backupCodes = JSON.parse(symmetricDecrypt(user.backupCodes, process.env.CALENDSO_ENCRYPTION_KEY)); | ||
|
|
||
| // check if user-supplied code matches one | ||
| const index = backupCodes.indexOf(req.body.backupCode.replaceAll("-", "")); | ||
| if (index === -1) { | ||
| return res.status(400).json({ error: ErrorCode.IncorrectBackupCode }); |
There was a problem hiding this comment.
Normalize backup code input before lookup.
Users often paste codes with whitespace or uppercase; without normalization, valid codes can be rejected.
🧹 Suggested fix
- const index = backupCodes.indexOf(req.body.backupCode.replaceAll("-", ""));
+ const normalizedBackupCode = req.body.backupCode.replaceAll("-", "").trim().toLowerCase();
+ const index = backupCodes.indexOf(normalizedBackupCode);🤖 Prompt for AI Agents
In `@apps/web/pages/api/auth/two-factor/totp/disable.ts` around lines 58 - 63,
Normalize the incoming backup code before lookup: take req.body.backupCode, trim
surrounding whitespace, remove internal whitespace and dashes (e.g., replace
spaces and "-" characters), convert to the same case used in stored backupCodes
(e.g., toLowerCase()), then check indexOf against backupCodes; update the lookup
that currently does backupCodes.indexOf(req.body.backupCode.replaceAll("-", ""))
to use this normalized value and keep the existing error return
(ErrorCode.IncorrectBackupCode) unchanged.
Comment on lines
+50
to
54
| // FIXME: this passes even when switch is not checked, compare to test | ||
| // below which checks for data-state="checked" and works as expected | ||
| await page.waitForSelector(`[data-testid=two-factor-switch]`); | ||
| await expect(page.locator(`[data-testid=two-factor-switch]`).isChecked()).toBeTruthy(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && find . -name "login.2fa.e2e.ts" -type fRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 145
🏁 Script executed:
cat -n apps/web/playwright/login.2fa.e2e.ts | head -70 | tail -30Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1288
🏁 Script executed:
rg "\.isChecked\(\)" --type ts --type js -B 2 -A 2Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 532
🏁 Script executed:
rg "toHaveAttribute.*data-state" --type ts --type js -B 1 -A 1Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 72
🏁 Script executed:
rg "data-state.*checked" apps/web/playwright/login.2fa.e2e.ts -B 3 -A 3Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1704
🌐 Web query:
Playwright locator.isChecked() Promise expect toBeTruthy behavior
💡 Result:
locator.isChecked()returns aPromise<boolean>(it resolves totrue/false, and throws if the element isn’t a checkbox/radio). So you mustawaitit before using a generic matcher liketoBeTruthy(). If you doexpect(locator.isChecked()).toBeTruthy()withoutawait, you’re asserting that a Promise object is truthy (it is), not that the checkbox is checked. [1]- Also,
expect(await locator.isChecked()).toBeTruthy()is a one-time check and does not auto-wait/retry, so it can be flaky if the UI updates asynchronously. Playwright recommends using the locator assertion instead:await expect(locator).toBeChecked(), which auto-retries until the expect timeout. [1][2][3]
Recommended
await expect(page.getByRole('checkbox', { name: 'Subscribe' })).toBeChecked();If you really want isChecked()
expect(await locator.isChecked()).toBeTruthy(); // no auto-waitSources: [1] Playwright locator.isChecked() docs, [2] Playwright expect(locator).toBeChecked() docs, [3] Playwright assertions (generic vs auto-retrying async assertions).
Fix the 2FA switch assertion — the Promise is never awaited.
The assertion expect(page.locator('[data-testid=two-factor-switch]').isChecked()).toBeTruthy() passes on the Promise object itself (which is always truthy) rather than the resolved boolean value. Since isChecked() returns Promise<boolean>, it must be awaited before assertion. Use a Playwright matcher on the locator instead, which also provides auto-retry behavior:
- await expect(page.locator(`[data-testid=two-factor-switch]`).isChecked()).toBeTruthy();
+ await expect(page.locator(`[data-testid=two-factor-switch]`)).toHaveAttribute(
+ "data-state",
+ "checked"
+ );This approach aligns with the working pattern elsewhere in this test file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // FIXME: this passes even when switch is not checked, compare to test | |
| // below which checks for data-state="checked" and works as expected | |
| await page.waitForSelector(`[data-testid=two-factor-switch]`); | |
| await expect(page.locator(`[data-testid=two-factor-switch]`).isChecked()).toBeTruthy(); | |
| // FIXME: this passes even when switch is not checked, compare to test | |
| // below which checks for data-state="checked" and works as expected | |
| await page.waitForSelector(`[data-testid=two-factor-switch]`); | |
| await expect(page.locator(`[data-testid=two-factor-switch]`)).toHaveAttribute( | |
| "data-state", | |
| "checked" | |
| ); |
🤖 Prompt for AI Agents
In `@apps/web/playwright/login.2fa.e2e.ts` around lines 50 - 54, The current
assertion uses page.locator('[data-testid=two-factor-switch]').isChecked() which
returns a Promise<boolean> and is not awaited, so the test asserts the Promise
object itself; change the assertion to either await the boolean (await
page.locator('[data-testid=two-factor-switch]').isChecked() and assert the
result) or — preferably — use the Playwright locator matcher on the locator
(e.g., expect(page.locator('[data-testid=two-factor-switch]')).toBeChecked()) to
get proper awaiting and auto-retry behavior.
Comment on lines
+47
to
+51
| <button | ||
| className="text-emphasis h-9" | ||
| tabIndex={-1} | ||
| type="button" | ||
| onClick={() => toggleIsPasswordVisible()}> |
There was a problem hiding this comment.
Restore keyboard access to the password visibility toggle.
tabIndex={-1} removes the button from the tab order, so keyboard users can’t toggle password visibility.
✅ Suggested fix
- tabIndex={-1}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <button | |
| className="text-emphasis h-9" | |
| tabIndex={-1} | |
| type="button" | |
| onClick={() => toggleIsPasswordVisible()}> | |
| <button | |
| className="text-emphasis h-9" | |
| type="button" | |
| onClick={() => toggleIsPasswordVisible()}> |
🤖 Prompt for AI Agents
In `@packages/ui/components/form/inputs/Input.tsx` around lines 47 - 51, The
password visibility toggle button in Input.tsx is currently removed from
keyboard focus by tabIndex={-1}; update the button (the element that calls
toggleIsPasswordVisible()) to be keyboard-focusable by removing the tabIndex or
setting it to 0, and ensure it has an accessible name (e.g., aria-label or
visible text) so keyboard and screen-reader users can operate the toggle.
Sign in
to join this conversation on GitHub.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features